[testcore] Shared cluster pool for clusters that require WorkerService#10905
[testcore] Shared cluster pool for clusters that require WorkerService#10905long-nt-tran wants to merge 5 commits into
Conversation
64ac31e to
db86e1e
Compare
db86e1e to
bdd1214
Compare
| } | ||
|
|
||
| func TestPollerScalingFunctionalSuite(t *testing.T) { | ||
| testcore.UseSuiteScopedCluster(t) //nolint:staticcheck // SA1019: suite reuses one worker-service cluster to avoid per-test cluster churn. |
There was a problem hiding this comment.
Can we delete UseSuiteScopedCluster now actually?
There was a problem hiding this comment.
I think we still need it until we split versioning_3 suite
| applyPoolSizeOverride(&sharedSize, "TEMPORAL_TEST_SHARED_CLUSTERS") | ||
|
|
||
| // System workers require significantly more memory, so default to a smaller pool. | ||
| sharedWorkerSize := max(1, runtime.GOMAXPROCS(0)/4) |
There was a problem hiding this comment.
Intuitively, I would have started using the same size as for regular clusters but I think this works, too. We can tweak it later. We'll need to update testing.md to include the new env var, though.
There was a problem hiding this comment.
sounds fine to me, let me keep it the same as other pool (to be more optimistic) and if we still OOM we can make it smaller
dnr
left a comment
dnr
left a comment
There was a problem hiding this comment.
FWIW: my opinion is that the worker service is an integral part of the server and should always be run as part of the server. Yes, it's true that many tests work fine without it, but using worker is not a bad smell or anything, it shouldn't require going out of your way or apologizing, or this complexity of multiple pools.
(At least until several years in the future when all internal workflows can be ported to chasm)
| // Doing a no-op global metric capture to satisfy the dedicated cluster guard, otherwise | ||
| // we'll trigger a cluster misuse fatal. | ||
| env.StartGlobalMetricCapture() |
There was a problem hiding this comment.
@stephanos using the WithMTLS() option causes more leaks, i spent a few tokens:
WithMTLS() is the cause. When the cluster tears down with mTLS enabled, the TLS connection becomes invalid. The sharedNamespaceWorker heartbeat RPC fails with a TLS error instead of a clean connection reset. TLS errors trigger a different
retry classification in the grpc middleware — the goroutine parks in waitRetryBackoff for a longer, more deterministic backoff window. By the time goleak samples the stacks, the goroutine is reliably mid-retry.
Without mTLS, the connection closes more cleanly — the goroutine either finishes the heartbeat or quickly returns to the idle (*sharedNamespaceWorker).run select loop, which IS in the ignore list. That race was nearly always won before.
waitRetryBackoff still needs to be in the ignore list regardless — it's a transient state of the already-documented "SDK worker goroutines not fully stopped" leak.
I didn't want to add more ignores, doing this for now. But can also see a case for adding MTLS + ignore options, and then work to address those in a future PR. LMK if that's preferable
There was a problem hiding this comment.
StartGlobalMetricCapture is good for now 👍 let's leave mTLS cleanup for after we cleaned up the current batch
@dnr I agree with you in principle, IMO for integration/e2e tests, the user generally shouldn't even need to care about what services are spun up or whether they require a dedicated, shared cluster, etc... (basically configs that user need to set when spinning up clusters should generally be related to their business logic) The mitigation here is mainly to deal with current testrunners hitting OOMs from spinning up too many "full clusters", without forcing test suites to run sequentially on a single cluster. Once we fix leaks when clusters are torn down + improving ergonomic of spinning up a test cluster, I want to get to a state where users don't actually need to care about needing to explicitly specifying these "backend config" options. cc @stephanos if you want to add more context |
3 participants
What changed?
Added a
sharedWithWorker, which is a reusable pool of test clusters for cluster setups that require the system workers.Why?
Right now, when a test requires system workers, we give it a dedicated cluster. However, this is heavy, and if we do this for all tests that require system workers, we risk a lot of OOM kills.
Technically speaking, reusing a cluster that has system workers is completely fine, so this PR does this by default (i.e., if we only enable system workers with no special configs, then we should aim to reuse this cluster).
For now, I'm creating a new shared pool of cluster-with-worker-service, rather than having a single shared pool, so we don't have to scan the entire pool to see which cluster is reusable.
How did you test it?
Potential risks
Also re-enabled system workers + parallelization on some existing tests that were made to be sequential to avoid OOMs, so I'll watch out for OOMs from those.